Skip to content

fix: dynamic Function/Generator/AsyncFunction constructor coercion gaps#476

Merged
dowdiness merged 7 commits into
mainfrom
fix/dynamic-constructor-coercion-gaps
Jun 27, 2026
Merged

fix: dynamic Function/Generator/AsyncFunction constructor coercion gaps#476
dowdiness merged 7 commits into
mainfrom
fix/dynamic-constructor-coercion-gaps

Conversation

@dowdiness

@dowdiness dowdiness commented Jun 26, 2026

Copy link
Copy Markdown
Owner

Summary

  • Gap 1 (GeneratorFunction/AsyncFunction coercion order): Fix param/body coercion to apply left-to-right per §20.2.1.1.1 step 5a, and use interp.to_js_string() (ES ToString) instead of MoonBit .to_string() for non-string args.

  • Gap 2 (GeneratorFunction yield-in-params): Detect YieldExpression in generator function params per §20.2.1.1.1 step 17a. Two-part fix: (a) push generator context before parsing params in parse_generator_decl so yield in defaults is parsed as YieldExpr not an identifier; (b) walk the AST via params_contain_yield to raise SyntaxError when found.

  • Gap 3 (Function .prototype.constructor wiring): Add .prototype object with back-pointer .constructor to dynamically created functions per §20.2.1.1.1 step 27a. Applied to FuncDecl, FuncDeclExt, and the fallback arm in builtins_function.mbt.

  • Gap 4 (AsyncFunction [[Prototype]]): Set AsyncFunction.[[Prototype]] = Function (the constructor) per §27.7.2, so Object.getPrototypeOf(AsyncFunction) === Function.

Test results

  • built-ins/GeneratorFunction: 42/42 (was 40/42 before; instance-yield-expr-in-param.js now passes in both modes)
  • built-ins/AsyncFunction: 34/34 (was 32/34; AsyncFunction-is-subclass.js now passes)
  • built-ins/Function: 796/824 (remaining failures are pre-existing issues unrelated to this PR)

Test plan

  • make test262-filter FILTER="built-ins/GeneratorFunction" → 42/42
  • make test262-filter FILTER="built-ins/AsyncFunction" → 34/34
  • No new built-ins/Function failures
  • CI green

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved generator and async function handling for more spec-compliant behavior.
    • Generator function parameters now correctly reject invalid yield usage in default values.
    • Dynamically created functions now expose more accurate prototype and constructor behavior.
    • Fixed parsing context handling so generator-related syntax is processed consistently and errors no longer leave the parser in a bad state.

dowdiness and others added 2 commits June 27, 2026 00:23
…rcion gaps

§20.2.1.1.1 CreateDynamicFunction — three gaps addressed:

1. GeneratorFunction/AsyncFunction: coerce params left-to-right (step 5a)
   and use interp.to_js_string() (ES ToString) for non-string arguments
   instead of MoonBit .to_string().

2. Function: add .prototype.constructor wiring to dynamically created
   functions so that `new (new Function(...))` correctly sets up the
   prototype chain per §20.2.1.1.1 step 27a.

3. AsyncFunction [[Prototype]]: set to the Function constructor per
   §27.7.2, so Object.getPrototypeOf(AsyncFunction) === Function.

4. GeneratorFunction params: add raw-source-text check for `yield` as
   a keyword in param strings (§20.2.1.1.1 step 17a). The parser does
   not enter generator context for params, so yield is parsed as an
   identifier; the keyword check is applied before parse.

All 42 GeneratorFunction and 34 AsyncFunction test262 tests now pass.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Per §15.5.1 of the ES spec, default parameter expressions in a
generator function's formal parameter list share the generator's
yield context. Push generator context before parsing params so
that `yield` in default values is correctly parsed as YieldExpr
rather than as an identifier.

This enables the AST-based §20.2.1.1.1 step 17a check to work
correctly via params_contain_yield(), replacing the previous
raw-text keyword scan which had false positives for `yield` in
comments, string literals, and object binding property keys.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@dowdiness, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 23 minutes and 24 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7687adb1-bfa4-432a-929a-db5ba11e9587

📥 Commits

Reviewing files that changed from the base of the PR and between 245bf2b and 6d78a70.

📒 Files selected for processing (5)
  • interpreter/runtime/generator.mbt
  • interpreter/stdlib/builtins_function.mbt
  • parser/early_errors.mbt
  • parser/expr.mbt
  • parser/stmt.mbt
📝 Walkthrough

Walkthrough

The PR updates generator constructor parsing to reject yield in parameter defaults, changes Function constructor outputs to create per-function prototype objects with constructor back-references, and aligns AsyncFunction prototype setup with the intrinsic Function constructor.

Changes

Constructor semantics updates

Layer / File(s) Summary
Generator parameter handling
parser/stmt.mbt, interpreter/runtime/generator.mbt
parse_generator_decl now shares generator context with parameter parsing and clears it on errors; generator helpers detect yield in parameter defaults, use interp.to_js_string(...) for constructor arguments, and reject generator parameters containing yield.
Function prototype objects
interpreter/stdlib/builtins_function.mbt
Function constructor paths now create per-function prototype objects, include them in the stamped function properties, and add constructor back-references for parsed and fallback function objects.
AsyncFunction prototype setup
interpreter/runtime/async.mbt
setup_async_function_constructor now resolves Function from the environment and updates the async constructor object’s prototype field when the constructor object is present.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • dowdiness/js_engine#26: The generator parameter yield handling and generator-context parsing changes continue the generator/yield work in that PR.
  • dowdiness/js_engine#405: The AsyncFunction prototype-chain update touches the same async constructor wiring area as that PR.

Poem

🐰 I hopped through constructors, neat and spry,
yield in the carrot patch got a stern “nope” reply.
Prototype burrows now have a proper door,
And async moonbeams point to Function once more.
Thump-thump, the engine hums so bright tonight.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: fixing construction and coercion gaps for dynamic Function, GeneratorFunction, and AsyncFunction.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/dynamic-constructor-coercion-gaps

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 245bf2b56c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread parser/stmt.mbt
// §15.5.1: params and body share generator context so that `yield` in
// default-parameter expressions is parsed as YieldExpression, enabling
// the §20.2.1.1.1 step 17a Contains check via the AST.
self.push_generator_context(true)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Allow yield as a destructuring property name

With the generator context pushed before parsing parameters, parse_params_ext() now also parses object binding-pattern property names under in_generator(). That path uses expect_ident(), so valid declarations such as function* g({yield: x}) {} (and dynamic GeneratorFunction("{yield: x}", "")) now throw even though yield is only a property name, not a binding identifier or a yield expression. The parser needs to keep rejecting yield bindings/expressions while still allowing keyword property names in binding patterns.

Useful? React with 👍 / 👎.

@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Benchmark Results

Run: https://github.com/dowdiness/js_engine/actions/runs/28278818103

startup/tiny_program is the PR #153 / issue #141 guardrail for built-in realm-stamping startup cost.

Stage summary

stage benchmarks total mean slowest benchmark slowest mean noisy rows
startup 3 2.193 ms startup/tiny_program 1.137 ms 0
frontend 7 0.932 ms pipeline/parse_heavy 0.514 ms 2
execution 26 14583.158 ms exec/fibonacci_30 13117.763 ms 2

Focused bytecode base-vs-head comparison

Base-vs-head deltas are reporting-only. Negative delta and PR/base < 1.00x mean the PR is faster; interpret high-CV or noisy rows cautiously.

benchmark stage base mean PR mean delta PR/base base CV PR CV noisy
baseline/bytecode/closure_factory execution 13.418 ms 13.092 ms -2.4% 0.98x 6.2% 8.0% no
pipeline/bytecode/evaluate execution 8.557 ms 9.053 ms +5.8% 1.06x 1.0% 4.5% no
isolate/bytecode/call_frame execution 7.562 ms 7.585 ms +0.3% 1.00x 5.3% 1.9% no
isolate/bytecode/runtime_helpers execution 10.714 ms 11.161 ms +4.2% 1.04x 0.5% 1.5% no
isolate/bytecode/local_access execution 35.903 ms 39.208 ms +9.2% 1.09x 0.9% 1.2% no
isolate/bytecode/env_access execution 36.064 ms 39.034 ms +8.2% 1.08x 1.0% 1.1% no
isolate/bytecode/captured_access execution 37.735 ms 37.495 ms -0.6% 0.99x 1.2% 1.7% no
isolate/bytecode/dispatch_stack execution 22.294 ms 23.823 ms +6.9% 1.07x 0.5% 1.6% no

Base-vs-head comparison

benchmark stage base mean PR mean delta PR/base base CV PR CV noisy
startup/tiny_program startup 1.324 ms 1.137 ms -14.1% 0.86x 8.1% 4.8% no
lexer/small frontend 0.037 ms 0.035 ms -5.5% 0.94x 45.6% 25.8% base, PR
lexer/large frontend 0.302 ms 0.303 ms +0.4% 1.00x 3.3% 1.0% no
exec/fibonacci_30 execution 13857.281 ms 13117.763 ms -5.3% 0.95x 0.4% 0.6% no
exec/property_chain execution 15.677 ms 15.305 ms -2.4% 0.98x 9.6% 13.5% no
startup/phase/parse_tiny frontend 0.002 ms 0.002 ms -0.8% 0.99x 0.6% 0.3% no
startup/phase/new_interpreter startup 1.232 ms 1.055 ms -14.4% 0.86x 6.3% 9.6% no
startup/phase/execute_preparsed_tiny execution 0.001 ms 0.001 ms -8.2% 0.92x 0.9% 0.6% no
startup/phase/event_loop_drain_empty startup 0.000 ms 0.000 ms -7.5% 0.92x 1.3% 0.9% no
startup/phase/result_stringify_output execution 0.000 ms 0.000 ms -2.2% 0.98x 0.5% 0.5% no
exec/array_map_filter execution 20.443 ms 20.013 ms -2.1% 0.98x 20.6% 20.1% base, PR
exec/closure_factory execution 30.861 ms 29.365 ms -4.8% 0.95x 7.6% 9.0% no
baseline/closure_legacy/closure_factory execution 28.918 ms 27.197 ms -6.0% 0.94x 8.8% 8.8% no
baseline/bytecode/closure_factory execution 13.418 ms 13.092 ms -2.4% 0.98x 6.2% 8.0% no
isolate/bytecode/dispatch_stack execution 22.294 ms 23.823 ms +6.9% 1.07x 0.5% 1.6% no
isolate/bytecode/local_access execution 35.903 ms 39.208 ms +9.2% 1.09x 0.9% 1.2% no
isolate/bytecode/env_access execution 36.064 ms 39.034 ms +8.2% 1.08x 1.0% 1.1% no
isolate/bytecode/captured_access execution 37.735 ms 37.495 ms -0.6% 0.99x 1.2% 1.7% no
isolate/bytecode/call_frame execution 7.562 ms 7.585 ms +0.3% 1.00x 5.3% 1.9% no
isolate/bytecode/runtime_helpers execution 10.714 ms 11.161 ms +4.2% 1.04x 0.5% 1.5% no
isolate/bytecode/property_get execution 43.978 ms 44.199 ms +0.5% 1.01x 2.2% 2.2% no
isolate/bytecode/property_set execution 41.764 ms 42.564 ms +1.9% 1.02x 0.6% 0.8% no
isolate/bytecode/method_call execution 8.318 ms 8.485 ms +2.0% 1.02x 0.6% 0.4% no
isolate/bytecode/object_literal execution 13.316 ms 13.924 ms +4.6% 1.05x 0.9% 2.2% no
isolate/bytecode/array_literal execution 14.899 ms 14.604 ms -2.0% 0.98x 3.6% 1.3% no
exec/for_of execution 6.179 ms 5.930 ms -4.0% 0.96x 14.2% 8.0% no
exec/arithmetic_loop execution 1022.939 ms 1005.261 ms -1.7% 0.98x 0.9% 0.3% no
exec/object_construction execution 7.568 ms 6.906 ms -8.8% 0.91x 7.6% 5.6% no
exec/string_ops execution 1.961 ms 1.897 ms -3.3% 0.97x 31.4% 23.0% base, PR
pipeline/exec/lex frontend 0.030 ms 0.031 ms +5.3% 1.05x 0.9% 1.2% no
pipeline/exec/parse frontend 0.028 ms 0.027 ms -3.8% 0.96x 3.3% 3.0% no
pipeline/exec/evaluate execution 27.888 ms 25.264 ms -9.4% 0.91x 10.8% 5.9% no
pipeline/closure_legacy/evaluate execution 25.235 ms 24.033 ms -4.8% 0.95x 5.1% 5.3% no
pipeline/bytecode/compile frontend 0.021 ms 0.021 ms -2.3% 0.98x 23.1% 27.4% base, PR
pipeline/bytecode/evaluate execution 8.557 ms 9.053 ms +5.8% 1.06x 1.0% 4.5% no
pipeline/parse_heavy frontend 0.500 ms 0.514 ms +2.8% 1.03x 4.0% 6.8% no

Mean-time chart (log scale)

benchmark stage mean chart
startup/tiny_program startup 1.137 ms ##
lexer/small frontend 0.035 ms ⚠ #
lexer/large frontend 0.303 ms #
exec/fibonacci_30 execution 13117.763 ms ##############################
exec/property_chain execution 15.305 ms ########
startup/phase/parse_tiny frontend 0.002 ms #
startup/phase/new_interpreter startup 1.055 ms ##
startup/phase/execute_preparsed_tiny execution 0.001 ms #
startup/phase/event_loop_drain_empty startup 0.000 ms #
startup/phase/result_stringify_output execution 0.000 ms #
exec/array_map_filter execution 20.013 ms ⚠ #########
exec/closure_factory execution 29.365 ms ##########
baseline/closure_legacy/closure_factory execution 27.197 ms ##########
baseline/bytecode/closure_factory execution 13.092 ms ########
isolate/bytecode/dispatch_stack execution 23.823 ms ##########
isolate/bytecode/local_access execution 39.208 ms ###########
isolate/bytecode/env_access execution 39.034 ms ###########
isolate/bytecode/captured_access execution 37.495 ms ###########
isolate/bytecode/call_frame execution 7.585 ms ######
isolate/bytecode/runtime_helpers execution 11.161 ms #######
isolate/bytecode/property_get execution 44.199 ms ############
isolate/bytecode/property_set execution 42.564 ms ###########
isolate/bytecode/method_call execution 8.485 ms #######
isolate/bytecode/object_literal execution 13.924 ms ########
isolate/bytecode/array_literal execution 14.604 ms ########
exec/for_of execution 5.930 ms ######
exec/arithmetic_loop execution 1005.261 ms #####################
exec/object_construction execution 6.906 ms ######
exec/string_ops execution 1.897 ms ⚠ ###
pipeline/exec/lex frontend 0.031 ms #
pipeline/exec/parse frontend 0.027 ms #
pipeline/exec/evaluate execution 25.264 ms ##########
pipeline/closure_legacy/evaluate execution 24.033 ms ##########
pipeline/bytecode/compile frontend 0.021 ms ⚠ #
pipeline/bytecode/evaluate execution 9.053 ms #######
pipeline/parse_heavy frontend 0.514 ms #

Closure-conversion comparison

  • unavailable

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
parser/stmt.mbt (1)

188-211: 🩺 Stability & Availability | 🟡 Minor

Extended error recovery path leaks context on secondary parsing failures.

The generator/async contexts are pushed at lines 9–10. If parse_params() fails with a specific error (e.g., "REST_PARAM_FOUND"), the code attempts recovery by calling parse_params_ext() and parse_block_body() (lines 17–19). However, if either of these calls raises an exception, the explicit pop operations (lines 20–21) are bypassed, and the exception propagates.

The surrounding catch block only wraps the initial parse_params() call; it does not guard the recovery logic inside the Failure arm. Consequently, any failure in the recovery path leaves the contexts pushed at lines 9–10 on the stack.

The standard success path and the initial parse_params failure paths correctly balance pushes and pops, but this asymmetry corrupts the parser's context stack depth during error recovery.

@@ -16,16 +16,23 @@ fn Parser::parse_generator_decl(self : Parser) -> `@ast.Stmt` raise Error {
       if msg == "REST_PARAM_FOUND" ||
         msg == "DEFAULT_PARAM_FOUND" ||
         msg == "DESTRUCTURE_PARAM_FOUND" {
         self.pos = saved_pos
-        let (ext_params, rest_param) = self.parse_params_ext()
-        // Context already pushed; parse body within the same context.
-        let body = self.parse_block_body()
-        self.pop_async_context()
-        self.pop_generator_context()
-        return GeneratorDeclExt(
-          name,
-          ext_params,
-          rest_param,
-          body,
-          tok.loc,
-          self.consume_source_text(tok.loc.offset),
-        )
+        // Guard recovery calls to ensure context cleanup on error.
+        try {
+          let (ext_params, rest_param) = self.parse_params_ext()
+          let body = self.parse_block_body()
+          return GeneratorDeclExt(
+            name,
+            ext_params,
+            rest_param,
+            body,
+            tok.loc,
+            self.consume_source_text(tok.loc.offset),
+          )
+        }
+        // Ensure contexts are popped if recovery fails.
+        finally {
+          self.pop_async_context()
+          self.pop_generator_context()
+        }
       } else {
         self.pop_async_context()
         self.pop_generator_context()
         raise `@errors.SyntaxError`(message=msg)
       }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@parser/stmt.mbt` around lines 188 - 211, The recovery branch in the
generator/async parsing flow leaves contexts pushed if parse_params_ext() or
parse_block_body() throws, so update the parse logic in stmt.mbt to ensure
pop_async_context() and pop_generator_context() run for every recovery attempt.
Wrap the recovery work inside the same GeneratorDeclExt path with a cleanup
guard or nested catch/finally-style handling so the contexts are always unwound
before re-raising, and verify both the success and failure paths in the
parse_params()/parse_params_ext() flow stay balanced.
🧹 Nitpick comments (1)
interpreter/stdlib/builtins_function.mbt (1)

425-440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider extracting the repeated prototype/back-reference setup.

The FuncDecl, FuncDeclExt, and fallback paths now duplicate the same prototype object, "prototype" descriptor, and "constructor" descriptor wiring. A small helper would keep these spec-sensitive attributes synchronized if one branch changes later.

Also applies to: 490-503, 507-522, 569-582, 588-646

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@interpreter/stdlib/builtins_function.mbt` around lines 425 - 440, The
prototype/back-reference wiring is duplicated across the FuncDecl, FuncDeclExt,
and fallback paths, so extract that repeated setup into a shared helper. Move
the common creation of the prototype object plus the "prototype" and
"constructor" descriptors into one place, and have the affected branches call it
from builtins_function.mbt so the spec-sensitive attributes stay consistent if
one branch changes. Use the existing function/class symbols around the
function-creation flow to centralize this logic without altering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@interpreter/runtime/generator.mbt`:
- Around line 364-371: The generator-parameter yield validation in
GeneratorDeclExt is only catching the current AST shape and misses destructuring
and computed-key parameter forms that can also contain yield expressions. Update
params_contain_yield to recursively inspect all parameter patterns used by
generator params, or add equivalent validation in the GeneratorDeclExt handling
so every nested default, destructuring, and computed-key case is rejected
consistently before codegen.
- Around line 176-185: params_contain_yield only checks Param.default_val, so it
misses yield expressions nested inside destructuring patterns. Update this
verifier in generator.mbt to also traverse p.pattern and recurse through
ArrayPat/ObjectPat trees, checking DefaultPat expressions, PropPat.default_val,
and PropPat.computed_key with expr_contains_yield. Keep the existing top-level
default_val check, but add pattern traversal so GeneratorFunction cases like
destructured defaults and computed keys are rejected as SyntaxError.

---

Outside diff comments:
In `@parser/stmt.mbt`:
- Around line 188-211: The recovery branch in the generator/async parsing flow
leaves contexts pushed if parse_params_ext() or parse_block_body() throws, so
update the parse logic in stmt.mbt to ensure pop_async_context() and
pop_generator_context() run for every recovery attempt. Wrap the recovery work
inside the same GeneratorDeclExt path with a cleanup guard or nested
catch/finally-style handling so the contexts are always unwound before
re-raising, and verify both the success and failure paths in the
parse_params()/parse_params_ext() flow stay balanced.

---

Nitpick comments:
In `@interpreter/stdlib/builtins_function.mbt`:
- Around line 425-440: The prototype/back-reference wiring is duplicated across
the FuncDecl, FuncDeclExt, and fallback paths, so extract that repeated setup
into a shared helper. Move the common creation of the prototype object plus the
"prototype" and "constructor" descriptors into one place, and have the affected
branches call it from builtins_function.mbt so the spec-sensitive attributes
stay consistent if one branch changes. Use the existing function/class symbols
around the function-creation flow to centralize this logic without altering
behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7f47893d-f629-4173-8e46-1469856bc354

📥 Commits

Reviewing files that changed from the base of the PR and between ec4b801 and 245bf2b.

📒 Files selected for processing (4)
  • interpreter/runtime/async.mbt
  • interpreter/runtime/generator.mbt
  • interpreter/stdlib/builtins_function.mbt
  • parser/stmt.mbt

Comment thread interpreter/runtime/generator.mbt
Comment thread interpreter/runtime/generator.mbt
… params

Four targeted fixes from review feedback on PR #476:

1. parse_object_pattern: use expect_ident_name() for property keys so that
   `yield` is accepted as a LiteralPropertyName (ES §12.2.6) even in generator
   context. Fixes GF("{yield: x}", "return x") incorrectly throwing SyntaxError.

2. parse_generator_decl: wrap parse_params_ext() and parse_block_body() calls in
   inline catch blocks so push_generator_context / push_async_context are always
   popped on error, eliminating the parser context-stack leak.

3. pattern_contains_yield: new helper that recursively walks ArrayPat, ObjectPat,
   DefaultPat, and PropPat (computed_key + default_val) so that yield expressions
   nested inside destructuring defaults (e.g. {x = yield} or {[yield]: x}) are
   correctly caught. Property name strings are intentionally excluded.

4. builtins_function.mbt: extract make_function_prototype / wire_constructor_to_prototype
   helpers to deduplicate the identical prototype-object creation and
   constructor back-reference wiring across the FuncDecl, FuncDeclExt, and
   fallback return paths.

GeneratorFunction: 42/42 test262, 2249/2249 unit tests.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e13e186b7b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread parser/stmt.mbt
// §15.5.1: params and body share generator context so that `yield` in
// default-parameter expressions is parsed as YieldExpression, enabling
// the §20.2.1.1.1 step 17a Contains check via the AST.
self.push_generator_context(true)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reset generator context for nested function params

Because this context is now pushed before parsing the outer parameter list, it also leaks into default-value subexpressions. parse_func_expr() and object-method parsing do not reset generator context until after their own parameters, so valid code such as GeneratorFunction("a = function(yield) {}", "") or GeneratorFunction("a = {m(yield){}}", "") now throws while parsing the nested non-generator parameter even though that nested function has its own yield context.

Useful? React with 👍 / 👎.

Comment thread parser/stmt.mbt Outdated
Comment thread interpreter/runtime/generator.mbt Outdated
…g guard, class yield traversal

Three follow-up fixes from second review round on PR #476:

1. parse_func_expr + object method shorthand: push push_generator_context(false)
   and push_async_context before parse_params() (not just before parse_block_body())
   so that nested non-generator params are isolated from an outer generator's
   context. Valid code such as GF("a = function(yield){}", "") or
   GF("a = {m(yield){}}", "") no longer throws. Context cleanup added for all
   error paths (parse_params_ext, parse_block_body) via inline catch.

2. parse_object_pattern: split the fall-through arm on peek_kind_at(1) == Colon
   so that property-key positions ({yield: x}) use expect_ident_name() while
   shorthand BindingIdentifier positions ({yield}) revert to expect_ident() and
   correctly reject reserved words. Fixes regression from the previous commit
   where {if}/{yield} shorthand was silently accepted.

3. expr_contains_yield: ClassExpr is NOT a function boundary for
   Contains(YieldExpression): the heritage expression (extends clause) and
   computed member-key expressions (class { [yield](){} }) are evaluated in the
   enclosing scope (ES §15.7.1). Only the method bodies are a new boundary.
   Split ClassExpr off the false arm and recurse into superclass + computed keys.

GeneratorFunction: 42/42 test262, 2249/2249 unit tests.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bcc32f70cb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread parser/stmt.mbt
Comment on lines +181 to +182
self.push_generator_context(true)
self.push_async_context(false)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reset context before async function params

Because this generator context is now active while parsing parameter default expressions, nested async function expressions still parse their own parameters under in_generator(): parse_async_func_body only pushes its new context after parse_params/parse_params_ext. Valid inputs such as GeneratorFunction("a = async function(yield) {}", "") therefore throw while parsing the nested non-generator async function parameter, even though only the outer generator parameter list should reject yield expressions. Fresh evidence relative to the earlier context-leak comment: the current patch fixed parse_func_expr and object methods, but the async-function helper still parses params before resetting context.

Useful? React with 👍 / 👎.

…c_func_body

parse_async_func_body pushed push_generator_context / push_async_context only
after parse_params(), so a nested async function expression inside an outer
generator's default parameter (e.g. GF("a = async function(yield) {}", ""))
parsed its parameter list under the outer generator context, making `yield`
a reserved keyword and incorrectly throwing SyntaxError.

Move both push calls to before parse_params(), mirroring the fix already
applied to parse_func_expr and object-method shorthand in the previous commit.
Error-path cleanup (pop on all raise sites) added for parse_params_ext and
parse_block_body branches.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4e265c0e17

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread interpreter/runtime/generator.mbt Outdated
Comment on lines +165 to +169
| AsyncArrowFuncExt(_, _, _, _, _)
| AsyncGeneratorExpr(_, _, _, _, _)
| AsyncGeneratorExprExt(_, _, _, _, _, _)
| ArrowFunc(_, _, _, _)
| ArrowFuncExt(_, _, _, _, _) => false

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Traverse arrow parameter initializers for yield

When a generator-constructor parameter default contains an arrow with its own default parameter, e.g. GeneratorFunction("x = ((a = yield) => {})", ""), the parser now records that inner default as a YieldExpr, but this branch treats the whole ArrowFuncExt as a boundary and params_contain_yield therefore accepts it. Arrow parameter initializers are still covered by the outer FormalParameters Contains(YieldExpression) check, so this needs to inspect ArrowFuncExt/AsyncArrowFuncExt params rather than returning false for the entire node.

Useful? React with 👍 / 👎.

Comment thread interpreter/runtime/generator.mbt Outdated
TaggedTemplate(tag, _, exprs, _) =>
expr_contains_yield(tag) ||
exprs.iter().any(fn(e) { expr_contains_yield(e) })
DestructureAssign(_, rhs, _) => expr_contains_yield(rhs)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Scan destructuring assignment patterns for yield

For parameter defaults that contain a destructuring assignment, this only scans the RHS and misses yield expressions embedded in the assignment pattern. Inputs such as GeneratorFunction("x = ({[yield]: a} = {})", "") or GeneratorFunction("x = ({a = yield} = {})", "") will build a DestructureAssign whose pattern contains the YieldExpr, but the new step-17a check lets it through instead of throwing the required SyntaxError.

Useful? React with 👍 / 👎.

Two gaps in expr_contains_yield / params_contain_yield:

1. ArrowFuncExt / AsyncArrowFuncExt were treated as full function boundaries and
   returned false for the whole node. Arrow parameter default values are evaluated
   in the enclosing scope (not the arrow's body scope), so they are covered by the
   outer FormalParameters Contains(YieldExpression) check. Split them out of the
   false group and delegate to params_contain_yield, which already walks
   default_val and nested patterns.
   e.g. GF("x = ((a = yield) => {})", "") now throws SyntaxError.

2. DestructureAssign only scanned the RHS expression and skipped the LHS pattern.
   Computed keys ({[yield]: a}) and property defaults ({a = yield}) in the pattern
   are also in the enclosing scope. Extend the arm to call pattern_contains_yield
   on the pattern as well.
   e.g. GF("x = ({[yield]: a} = {})", "") now throws SyntaxError.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b63b7b317a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread parser/stmt.mbt
// §15.5.1: params and body share generator context so that `yield` in
// default-parameter expressions is parsed as YieldExpression, enabling
// the §20.2.1.1.1 step 17a Contains check via the AST.
self.push_generator_context(true)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject yield in generator declaration parameters

Pushing generator context here affects ordinary function* declarations too, but the new params_contain_yield early error is only run in the dynamic GeneratorFunction constructor. As a result, source like function* g(a = yield 1) {} now parses as a GeneratorDeclExt and can pass the existing block/signature validation, even though generator formal parameters containing a YieldExpression must be a SyntaxError.

Useful? React with 👍 / 👎.

Comment thread parser/expr.mbt
// Push context before params so nested async-function params are isolated
// from any outer generator's context (mirrors fix in parse_func_expr).
self.push_generator_context(is_generator)
self.push_async_context(true)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject await in async function expression parameters

This makes await parse as an AwaitExpr while reading async function expression parameters, but the later signature/default validation only recurses through AwaitExpr and never applies the async-formal-parameters Contains(AwaitExpression) early error. For example, (async function(a = await 1) {}) is now accepted instead of throwing the required SyntaxError.

Useful? React with 👍 / 👎.

Comment thread parser/expr.mbt
// Push context before params so method params see a clean scope,
// isolating them from any outer generator's context.
self.push_generator_context(false)
self.push_async_context(obj_is_async)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject await in async object method parameters

When obj_is_async is true, this parses async object-method parameter defaults under async context, but object method signature validation also lacks a Contains(AwaitExpression) check. That lets invalid syntax such as ({ async m(a = await 1) {} }) build an AsyncFuncExprExt instead of raising the required SyntaxError for await in async formal parameters.

Useful? React with 👍 / 👎.

Three source-level early errors that were only enforced by the dynamic
GeneratorFunction/AsyncFunction constructors, not by the parser for
regular source code:

1. Generator declaration formal parameters containing YieldExpression
   (function* g(a = yield 1) {}): add params_contain_yield() check after
   parse_params_ext() in parse_generator_decl. The generator context was
   already pushed before params so yield 1 parses as YieldExpr; only the
   early-error raise was missing.

2. Async function expression formal parameters containing AwaitExpression
   (async function(a = await 1) {}): add params_contain_await() check
   after parse_params_ext() in parse_async_func_body. Also adds
   params_contain_yield() for the async generator expression case.

3. Async object method formal parameters containing AwaitExpression
   ({ async m(a = await 1) {} }): add params_contain_await() check after
   parse_params_ext() in the object-method-shorthand ext-params path.

All three checks live in the new parser/early_errors.mbt file which
provides expr_has_yield, pat_has_yield, params_contain_yield and their
_await counterparts. These mirror interpreter/runtime/generator.mbt
so the parser can enforce the ES §15.5.1 / §15.8.1 static semantics
without importing from the interpreter layer.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6d78a708c9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread parser/early_errors.mbt
.any(fn(mbr) {
match mbr {
Method(cm) => cm.computed && expr_has_await(cm.key)
Field(cf) => cf.computed && expr_has_await(cf.key)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject await in class field initializers in async params

When an async formal-parameter default contains a class expression with a field initializer, e.g. (async function(a = class { x = await 1 }) {}) or ({ async m(a = class { x = await 1 }) {} }), the newly active async context parses await 1 as an AwaitExpr. This class walker only checks heritage and computed keys, so params_contain_await returns false and the invalid async formal parameters are accepted instead of throwing; include field initializers in this check or prevent class field initializers from inheriting async context.

Useful? React with 👍 / 👎.

@dowdiness dowdiness merged commit 39c6bc1 into main Jun 27, 2026
15 checks passed
@dowdiness dowdiness deleted the fix/dynamic-constructor-coercion-gaps branch June 27, 2026 05:01
dowdiness added a commit that referenced this pull request Jun 27, 2026
ROADMAP.md: refresh test262 block to CI run 28279305916 (tip 39c6bc1,
2026-06-27). Key changes: strict 95.3% → 96.9%, non-strict 94.0% → 95.7%
P/E; ES2015 strict 95.9% → 97.7%, non-strict 95.7% → 97.5%.
Baseline minimums updated to 27,650 / 25,800 (was 23,520 / 22,450).

agent-todo.md: remove "Dynamic constructor coercion gaps" section — all
four items (ES ToString coercion, left-to-right order, [[Prototype]]
inheritance, error prototype chain) shipped in PR #476.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dowdiness added a commit that referenced this pull request Jun 27, 2026
* chore: ratchet test262 baseline after PR #476

non-strict: 27650 → 27674 (+24)
strict:     25800 → 25911 (+111)
combined:   53550 → 53685 (+135)

Source: CI run 28279305916 on main (post-merge of #476
fix: dynamic Function/Generator/AsyncFunction constructor coercion gaps),
buffer 100.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* docs: update test262 report and remove shipped constructor coercion todo

ROADMAP.md: refresh test262 block to CI run 28279305916 (tip 39c6bc1,
2026-06-27). Key changes: strict 95.3% → 96.9%, non-strict 94.0% → 95.7%
P/E; ES2015 strict 95.9% → 97.7%, non-strict 95.7% → 97.5%.
Baseline minimums updated to 27,650 / 25,800 (was 23,520 / 22,450).

agent-todo.md: remove "Dynamic constructor coercion gaps" section — all
four items (ES ToString coercion, left-to-right order, [[Prototype]]
inheritance, error prototype chain) shipped in PR #476.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant